Unfortunately I think this is a breaking change. Existing onOpenChange handlers aren't prepared to handle events other than MouseEvent, TouchEvent, or FocusEvent. The only way I can think of to make this not breaking is to create an onOpenChange2 event with the new type for the event arg.

Yeah good catch 👍, I tried playing with this a bit more in TS playground and it can indeed break typescript build depending on how users define their onOpenChange handlers https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBDAnmYcDyKB2AFCkBuwUAooZjAM5wC8AUHHAD5ynDn1NwCyEArhcFbsGzACp8AxgAshMWvKQo42KHirU4Abw4MImDGwDCUgIaYA5sABccABTX0WXASKyKAShoA+OPggBLABMAbloAX3kJPQp4PQNMYzNLGjsHHn5BMngxSRksz2ofbXDI6PgwVTAKGxU1FM04OKxEi1Qw0NogA

This is actually quite worrying because we will never be able to add new events to our callback types without breaking users 😱 - this looks like a good candidate to discuss in tech sync with the wider team

In this case we might want to consider casting internally in the Popover and Menu components to one of the existing event types, to unblock the fixing of the bugs in the description. It will change the runtime behaviour, but I think that is acceptable compared to the risk of build breaks for users.

However actually adding new events to types in useOnScrollOutside is also technically a breaking change in that case, even if the utility is not exported by our suite package 😱😱 - if we want full non-break coverage we should probably revert to @YuanboXue-Amber's original implementation and even cast the new event type within the utility.

All options are pretty bad for me, WDYT @behowell?

The original implementation is at this commit: b73b2bd

Casting to avoid a breaking change in types is still a breaking change; and possibly even worse: it results in extra-confusing runtime bugs when the object isn't the type that was promised. In this case, the event object would be missing any of the properties that are in MouseEvents.

I think in practice, it's pretty unlikely that it will cause an actual problem in this case, especially since the existing type includes React.FocusEvent<HTMLElement> and already requires you to do some runtime inspection of the object to see what type it is.

It might be worth discussing this with the rest of the team to come up with the right fix here. This isn't the first time this has caused a problem. In my recent PR #28951, I had to come up with a "hacky" workaround to this problem, by including the new event on the data argument instead. Unfortunately, that won't work here because it requires passing undefined for the event argument, which would be a breaking change here (the OpenPopoverEvents type doesn't include undefined as a possibility).

All of the possible options I can think of are broken in some way, so we'd have to decide what the least-bad option is:

1. Add | Event to the OpenPopoverEvents type (this PR).
   • Pros: Gets the type correct. Any potential breaks are caught at build time.
   • Cons: It's a breaking change for any code that explicitly types its event argument (instead of using our exported OpenPopoverEvents type).
2. Cast the event type to MouseEvent.
   • Pros: No type changes, and no changes to existing code to listen to the new case where onOpenChange can be called.
   • Cons: Still a breaking change, but the break happens at runtime instead of build time. The type is a lie.
3. Create onOpenChange2 with the new types, and deprecate onOpenChange.
   • Pros: No type breaks to existing code.
   • Cons: Breaks the contract of the old onOpenChange: it won't be called when the popover hides in response to a scrollbar. Code will need to be updated to onOpenChange2. Also, this adds a bunch of code to keep both the old event and new event working.

Given all of that, I think option 1 might actually be the least-bad option.

Yeah I think that 1. might be the least bad option, but it would require discussion with the wider team to make sure we know about it and the consequences. I would propose that we:

1. Implement option 2 (casting the event) to fix the standing bug
2. create an issue to track whether we stick to option 2 or switch to option 1
3. discuss the new issue in tech sync

In the interim option 2 is the best case IMO, since build breaks generally block our users from upgrading to fix other issues. I can't as easily think of cases where users rely on knowing exactly what the event type is. However for option 1 it's quite common from what I see in codebases to type the event

// 👍 no need to do anything
const onOpenChange: PopoverProps['onOpenChange'] = (e, data) => {/**/}

// 👎 breaking change - I've generally seen this more often since developers generally don't pick from PopoverProps as much
const onOpenChange = (e: React.MouseEvent | React.FocusEvent, data: PopoverOpenData) => {/**/}

I created a new PR #29062 using the cast option (option 2) to fix the original issue. Let's discuss how to extend event type this wednesday

Should this have /** @internal */ since it appears to only be used by an internal utility?

We could add it, but I'm not sure what the use of @internal is anymore, even if these utilties are internal we are still bound by the requirement to never break them until the next major, now that our API surface includes our entire set of packages 🥲

As far as I can tell, the goal here is to use UseOnClickOutsideOptions, except replace the callback field? If so, it'd be better to use Omit instead of Pick:

I'd personally prefer Pick since there's less risk of picking up any new properties unintentionally in the future. Since it's only types, there should be no consequences the extra code to pick explicit properties.

Pick will also break if any properties are removed - which Omit will not do https://www.typescriptlang.org/play?#code/C4TwDgpgBAYg9nAvFA3gKCpqAjAhgJwC4oA7AVwFtsJ8BuDLPAL2OwQBsJcT6BfNNKEhQAQgSjIA8hQCWwADzw4AGigByZmoB89QeGhimEqAAUZAYwDWihKo0E1UAD7qAZgm31MQA